Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Sample trace probabilistically #1228

Closed
wants to merge 2 commits into from
Closed

Conversation

shanson7
Copy link
Collaborator

@shanson7 shanson7 commented Mar 5, 2019

This change allows users to turn down trace sampling. Currently we are seeing span drops and don't need to trace every request we get.

Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Dieterbe
Copy link
Contributor

see #1055 for some prior (sparse) discussion.
I decided not to merge that PR and instead try to add config options to MT for all corresponding jaeger options.
This didn't go well because jaeger doesn't simply use a list of config values with corresponding defaults. certain defaults are being set after inspecting certain other variables
i also found some of the config options in jaeger not very well named, or there were some inconsistencies with booleans, i don't remember the details right now.
It's frustrating that this has been dragging on so long, and i want to merge this, but i want to look into this a bit more.
In particular, we can just give up on mapping directly to the jaeger options, but we should still lay out our options such that they support the different sampling mechanisms (we want to use the remote controlled one)

@shanson7
Copy link
Collaborator Author

That sounds fine but this is something we desperately needed on our side. Jaeger was dropping more spans than not, making it useless. So, it was either dedicate nearly 15% of our resources to jaeger, or sample a little bit (we sample 5%). It's worked out very well for us.

More config options will add complexity a little bit. I can't say I looked into all the options in jaeger, since probabilistic sampling was all I needed.

@Dieterbe
Copy link
Contributor

That sounds fine but this is something we desperately needed on our side

understood. we'll support the functionality, just want to have another look at a generic way to configure the jaeger client

@Dieterbe
Copy link
Contributor

Dieterbe commented Jun 4, 2019

i just submitted #1341 which I think is the better way to support this.

@Dieterbe Dieterbe closed this Jun 4, 2019
@shanson7 shanson7 deleted the sample_trace branch February 24, 2020 11:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants